-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add TranslateVarArray for simplified indexing #43
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
+ Coverage 88.25% 89.31% +1.05%
==========================================
Files 5 6 +1
Lines 315 393 +78
==========================================
+ Hits 278 351 +73
- Misses 37 42 +5
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice with higher efficiency! Should the new container just replace the existing IndexedVarArray? Or is it ok with two different types for a while?
Tables-support is not working for TranslateVarArray, but should be easy to fix.
Looks like I misplaced a comment somewhere.. just added support for Tables. I think we could use this as the default, but there is some overhead with the translation which I would like to remove/reduce. Also a Is it ok to keep as a separate type for now and see how it works for larger models and maybe cut down on the overhead? And then do a larger API overhaul for the next breaking release? There is a lot of overlap in the code between the two types, so a cleanup would be nice if we like this interface. Not super happy about the naming, though, any suggestions? (Both for |
I am fine with separate types for now. Not sure about better names... |
There is a considerable overhead when indexing on custom types. This PR investigates if we can relatively easily translate to a simpler and type stable value for indexing under the hood: